-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
various gcsweb updates #11176
various gcsweb updates #11176
Conversation
…duce manifest list
@@ -111,6 +116,20 @@ func main() { | |||
log.Fatal(http.ListenAndServe(fmt.Sprintf(":%d", *flPort), nil)) | |||
} | |||
|
|||
func upgradeToHTTPS(w http.ResponseWriter, r *http.Request, logger txnLogger) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side note: we should consider putting this in a util or finding one, we have a very similar method in deck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[not blocking for this PR, but something down the line maybe...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, no kidding.
test-infra/prow/cmd/deck/main.go
Lines 382 to 403 in c3f2665
// optionally inject http->https redirect handler when behind loadbalancer | |
if o.redirectHTTPTo != "" { | |
redirectMux := http.NewServeMux() | |
redirectMux.Handle("/", func(oldMux *http.ServeMux, host string) http.HandlerFunc { | |
return func(w http.ResponseWriter, r *http.Request) { | |
if r.Header.Get("x-forwarded-proto") == "http" { | |
redirectURL, err := url.Parse(r.URL.String()) | |
if err != nil { | |
logrus.Errorf("Failed to parse URL: %s.", r.URL.String()) | |
http.Error(w, "Failed to perform https redirect.", http.StatusInternalServerError) | |
return | |
} | |
redirectURL.Scheme = "https" | |
redirectURL.Host = host | |
http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently) | |
} else { | |
oldMux.ServeHTTP(w, r) | |
} | |
} | |
}(mux, o.redirectHTTPTo)) | |
mux = redirectMux | |
} |
I'm kinda impressed how I wrote almost exactly the same thing. (The deck implementation is cleverer by intercepting before any of the handler funcs are run, though. I couldn't quite figure out how to do that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that one's mine :+)
@@ -12,15 +12,15 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
FROM ARG_FROM | |||
FROM gcr.io/distroless/static | |||
|
|||
MAINTAINER Tim Hockin <thockin@google.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated but MAINTAINER
has long been deprecated, we just dropped these from the rest of our images in favor of OWNERS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm/approve
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, ixdy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
LGTM label has been added. Git tree hash: 07f937942896b7711d0ead5f9559b1d4929c41d9
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor points, mostly.
One option for protocol upgrade would be to write a tiny sidecar container that listens on port X, does protocol upgrade if needed, otherwise proxies to localhost port Y. That way, anyone could just throw that into a pod and not change their apps.
|
||
all-push: all-push-images push-manifest | ||
|
||
push-manifest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was (for a while) keeping this in sync with https://github.com/thockin/go-build-template which did a lot of what you have done here, but just epsilon differently. Would be nice to re-converge so fixes can be synced over time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I'd forgotten about that. can update to address.
all-push: all-push-images push-manifest | ||
|
||
push-manifest: | ||
docker manifest create --amend $(IMAGE):$(VERSION) $(shell echo $(ALL_ARCH) | sed -e "s~[^ ]*~$(IMAGE)\-&:$(VERSION)~g") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker manifest has bugs in some versions, which produce invalid manifests. Except docker proper didn;t validate manifests until recently. But containerd does...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been using this flow in k/k for a while without issue, though we do have guards over there in a few places to ensure that you're using docker 18.06.0 or newer (where the bug was fixed).
Google workstations all have new enough docker that I'm not super worried about this.
@@ -1,4 +1,4 @@ | |||
#!/bin/sh | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? does sh not exist? I would only say "bash" if we need bash-isms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The golang image links /bin/sh to dash, which doesn't support things like set -o pipefail
(apparently).
@@ -0,0 +1 @@ | |||
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just echo this in Dockerfile - one less file in git? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distroless image has no echo command. Also, running commands inside Dockerfiles gets trickier with multiarch (which is why I moved the chmod
to the Makefile).
Interesting - I don't see problems on git-sync. Will have to look..
…On Thu, Feb 7, 2019 at 3:51 PM Jeff Grafton ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In gcsweb/build/build.sh
<#11176 (comment)>
:
> @@ -1,4 +1,4 @@
-#!/bin/sh
+#!/usr/bin/env bash
The golang image links /bin/sh to dash, which doesn't support things like set
-o pipefail (apparently).
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#11176 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFVgVKqrN9Nb452NV5UM4YP7xJ_fPElmks5vLLvrgaJpZM4apXP3>
.
|
I was considering adding a separate nginx pod which did the upgrade, but I wasn't sure how to get the GCLB to forward to a different pod. I hadn't considered using an nginx container, though that also feels a little heavyweight (GCLB -> nginx -> gcsweb binary). might be worth looking into, though this approach here also works, so... 🤷 |
the |
gcr.io/distroless/static
(instead of alpine)/assign @thockin